JIT: Fold "shift-by-zero" in lower #61222
Conversation
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT Issue DetailsFixes a Checked-only assert found in #61209 by Fuzzlyn. cc @jakobbotsch
|
| @@ -5773,7 +5783,7 @@ void Lowering::LowerShift(GenTreeOp* shift) | |||
| assert(!cast->CastOp()->isContained()); | |||
|
|
|||
| // It has to be an upcast and CNS must be in [1..srcBits) range | |||
| if ((srcBits < dstBits) && ((UINT32)cns->IconValue() < srcBits)) | |||
| if ((srcBits < dstBits) && (cns->IconValue() > 0) && (cns->IconValue() < srcBits)) | |||
There was a problem hiding this comment.
Do I understand correctly this is the fix and the removal of nop shifts is a separate optimization, or is that required as well?
FWIW, I would prefer we not do these ad-hoc opts in lowering (instead tracing back the where the frontend "failed at its job" and so on).
There was a problem hiding this comment.
@SingleAccretion it's pretty normal to duplicate peepholes accross phases and we already do it. Front-end didn't fail here, only rationalizer managed to fold this:
N036 ( 47, 59) [000024] -ACXG------- \--* LSH int <l:$2c8, c:$2d2>
N035 ( 2, 5) [000067] -A--G------- \--* COMMA int
N033 ( 1, 3) [000042] -A--G---R--- +--* ASG ref <l:$1c2, c:$1d5>
N032 ( 1, 1) [000041] D------N---- | +--* LCL_VAR ref V02 tmp2 d:1 <l:$280, c:$85>
N031 ( 1, 1) [000060] ------------ | \--* LCL_VAR ref V03 cse0 u:1 <l:$280, c:$81>
N034 ( 1, 2) [000066] ------------ \--* CNS_INT int 0 <l:$2d1, c:$2d0>
into
N033 ( 1, 3) [000042] DA--G------- * STORE_LCL_VAR ref V02 tmp2 d:1
N034 ( 1, 2) [000066] ------------ t66 = CNS_INT int 0 <l:$2d1, c:$2d0>
/--* t9 int
+--* t66 int
N036 ( 47, 59) [000024] ---XG------- t24 = * LSH int <l:$2c8, c:$2d2>
/--* t24 int
N037 ( 48, 60) [000025] ---XG------- * RETURN int $107
There was a problem hiding this comment.
maybe some day we'll get a DSL for transformations which will be compiled into morph and lower at the same time (that's how some C++ compilers work today)
There was a problem hiding this comment.
Is it really worth to add this optimization in lower that only has hits in fuzzer generated code?
There was a problem hiding this comment.
@jakobbotsch not really, but the diff found 4 more cases and we still don't have collections for real-world apps to judge. but let me remove it since it raised concerns by you two
ghost
locked as resolved and limited conversation to collaborators
Fixes a Checked-only assert found in #61209 by Fuzzlyn.
cc @jakobbotsch
A few diffs:
e.g. https://www.diffchecker.com/oEoe5xQk